-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RichText: Do not run valueToEditableHTML on every render #12460
Conversation
Slightly related: #12402. |
@@ -874,7 +875,8 @@ export class RichText extends Component { | |||
tagName={ Tagname } | |||
onSetup={ this.onSetup } | |||
style={ style } | |||
defaultValue={ this.valueToEditableHTML( record ) } | |||
record={ record } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, wouldn't this have the same effect than before?
This is what I see:
- the
record
value is created on everyRichText
render by thegetRecord
function. - the
getRecord
function returns a new object every time is called. - on every
RichText
render, theTinyMCE
component will receive a new object reference for therecord
prop (although values may be the same, I haven't looked at what this function does), so it'll be re-rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I just saw that the TinyMCE
component defines a shouldComponentUpdate
that always returns false
. So what this PR is trying to avoid is to actually call valueToEditableHTML
in every RichText
render, not re-renders of the child component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, valueToEditableHTML
will only be called on mount there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, my understanding is that this would reduce the time it takes RichText to execute in every render/commit phase. I've tested this by:
- Opened the Gutenberg demo.
- In the first paragraph introduced the text "Howdy. ".
These are the results in master
:
These are the results in this branch:
Perhaps is difficult to observe in the GIFs, but it looks like RichText takes ~0,5ms to render in every keystroke in both master
and this branch. I don't see at the moment which other tests I could do to effectively measure the reduction of the performance budget introduced by this branch.
Nevertheless, doing less work is always better! So, I think it's fine to merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, within RichText
, the more expensive part seems to be the FormatEdit
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new record being created? Which function exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm from mobile now.
This is from React Developer Tools' new profiler (to be found in the React tab). You can navigate every commit and components that were updated will be colored (bluish to yellow, yellow the ones that took the most time).
The getRecord
method in RichText does:
getRecord(){
const { formats, text } = this.formatToValue( this.props.value );
const { start, end } = this.state;
return { formats, text, start, end };
}
essentially returning a new object no matter whether formats
, text
, start
, end
values have been updated or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the result of getRecord
get compared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see now. Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that React does a shallow comparison of the props passed to the components to decide whether a particular component needs to be re-rendered. We have two mechanisms to avoid re-rendering: 1) always pass the same prop (in the case of arrays and objects, the same reference) or 2) hook into shouldComponentUpdate
to do that process ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some e2e test failing, but they seem unrelated (may I have heard somewhere that they're now failing for every branch due to a Travis issue?).
This works properly and the code is fine, so giving you a 👍 dependant on e2e tests passing.
@nosolosw There are two render calls on every key press. In the picture below you see one selected, one a bit further on. Removing these calls speeds up the render call significantly. Overall it has a smaller impact on the whole work per key press though. |
6d4e876
to
9250199
Compare
9250199
to
a3326a4
Compare
Description
Currently we are calling
valueToEditableHTML
, which creates HTML from a RichText value, on every RichText render call, while this should only every be called once on every mount, to set the initial HTML in the TinyMCE component.I think this removes about 3.5-4% of the work done on every key press. Note that it does not only affect key presses, but renders in general. So this function will be called a lot more than needed.
How has this been tested?
Screenshots
Types of changes
Checklist: